Skip to content

Fixing files with few typing (mypy) errors #4263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

henrikmidtiby
Copy link
Contributor

Overview: What does this pull request change?

In this pull request I have addressed some of the files with only a few reported mypy errors.

Further Information and Comments

To identify files with few mypy errors, I cleared the mypy.ini file so no detected errors would be ignored and then used the follwing command

uv run pre-commit run mypy --all-files | grep "error" | sed 's/:.*//' | uniq -c | sort -n -r

I hope that this approach will make it easier to deal with the large files that needs a lot of type annotations added.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I left some change requests:

)

return Context.settings(
return_val: dict[str, Any] = Context.settings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a comment than a suggestion, but it's weird that MyPy doesn't detect that Context.settings() already returns a dict[str, Any]...

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Jun 22, 2025
henrikmidtiby and others added 10 commits June 22, 2025 22:24
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Cleaner way to indicate the float type

Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
@henrikmidtiby henrikmidtiby requested a review from chopan050 June 22, 2025 20:56
@henrikmidtiby
Copy link
Contributor Author

Thanks for your detailed comments. I have addressed all of the raised issues, in most cases by accepting your suggestions.

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I missed some last small things in mypy.ini. Could you please check them as well? After that, we should be ready to approve and merge this!

I also noticed that you modified a lot of files, but only some of them are indicated in mypy.ini for MyPy to stop ignoring their errors. Maybe you would like to add some more files in there? In particular, manim.animation.numbers, manim.scene.moving_camera_scene and manim.scene.section seem fine to me and MyPy didn't return any errors after adding them to mypy.ini, although manim.mobject.logo and the other animations you didn't add did throw some errors.

@@ -52,12 +52,24 @@ warn_return_any = True
ignore_errors = True
disable_error_code = return-value

[mypy-manim._config.cli_colors.*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since cli_colors is a file and not a directory, it's not necessary to end the tag with a .*.

Suggested change
[mypy-manim._config.cli_colors.*]
[mypy-manim._config.cli_colors]

Comment on lines +64 to +71
[mypy-manim.animation.changing.*]
ignore_errors = False

[mypy-manim.animation.fading.*]
ignore_errors = False

[mypy-manim.animation.updaters.update.*]
ignore_errors = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[mypy-manim.animation.changing.*]
ignore_errors = False
[mypy-manim.animation.fading.*]
ignore_errors = False
[mypy-manim.animation.updaters.update.*]
ignore_errors = False
[mypy-manim.animation.changing]
ignore_errors = False
[mypy-manim.animation.fading]
ignore_errors = False
[mypy-manim.animation.updaters.update]
ignore_errors = False

@@ -73,12 +85,21 @@ ignore_errors = True
[mypy-manim.mobject.*]
ignore_errors = True

[mypy-manim.mobject.text.code_mobject]
[mypy-manim.mobject.frame.*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[mypy-manim.mobject.frame.*]
[mypy-manim.mobject.frame]

Comment on lines +94 to +101
[mypy-manim.mobject.graphing.scale.*]
ignore_errors = False

[mypy-manim.mobject.text.code_mobject.*]
ignore_errors = False

[mypy-manim.mobject.three_d.three_d_utils.*]
ignore_errors = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[mypy-manim.mobject.graphing.scale.*]
ignore_errors = False
[mypy-manim.mobject.text.code_mobject.*]
ignore_errors = False
[mypy-manim.mobject.three_d.three_d_utils.*]
ignore_errors = False
[mypy-manim.mobject.graphing.scale]
ignore_errors = False
[mypy-manim.mobject.text.code_mobject]
ignore_errors = False
[mypy-manim.mobject.three_d.three_d_utils]
ignore_errors = False

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants